Conversation
| default = false; | ||
| visible = false; | ||
| apply = warn '' | ||
| Option `vim.visuals.fidget-nvim.setupOpts.integration.xcodebuild-nvim.enable` | ||
| has been deprecated upstream. Use | ||
| `vim.visuals.fidget-nvim.setupOpts.notification.window.avoid = ["TestExplorer"]` instead. | ||
| ''; |
There was a problem hiding this comment.
This is not the correct way to warn on an option. You'll want to use mkRemovedOptionModule as the rest of the codebase. Apply does work, but it's a really fragile way of doing this. Also applies to the vim.visuals.fidget-nvim.setupOpts.integration.nvim-tree.enable warning above.
There was a problem hiding this comment.
unfortunately not possible due to mkPluginSetupOption, see #1305
There was a problem hiding this comment.
sorry I lied xd, this throws a warning even if the option isn't set at all. We'll have to figure something out
NotAShelf
left a comment
There was a problem hiding this comment.
A few comments on formatting and introduction of new options. I'd appreciate it if you could follow the "convention" for mkOption.
Also, this requires a changelog entry after v0.9. LGTM otherwise. Though CC @horriblename for the HLS changes.
changelog for both commits? I can add it once the comments above are resolved |
horriblename
left a comment
There was a problem hiding this comment.
I'm pretty this will trigger the deprecation warning regardless of whether the user has set this option
I think removing the default value works? (I don't remember if it allows not giving a default without one)
| # see https://github.com/mrcjkb/haskell-tools.nvim/blob/v6.2.0/lua/haskell-tools/lsp/init.lua#L131-L173 | ||
| # for the real ones | ||
| hls = { | ||
| enable = false; |
There was a problem hiding this comment.
from my understanding, we're relying on haskell-tools to start the lsp, so we should set this to false? (enable = true, which is the default, calls vim.lsp.enable("server_name")
There was a problem hiding this comment.
enable is one of the fields not recognized by haskell-tools, as per :checkhealth:
Checking config ~
- ✅ OK No errors found in config.
- ⚠️ WARNING unrecognized configs in vim.g.haskell_tools: { "hls.enable", "hls.root_dir", "hls.filetypes" }
in that commect I left there, lsp_start_opts use some fields from hls_opts and hls_settings, which are the ones defined by us - there's no checks on enable
There was a problem hiding this comment.
just tested this on this branch, there's no warnings without enable and the LSP still works properly
There was a problem hiding this comment.
hmm in that case we should filter out enable before toLuaObject'ing it. I can do that in a separate PR, or you can do that in this one
There was a problem hiding this comment.
do you mean adding that enable back, setting it to true so that it stays consistent with the rest, but filtering that enable when passed to toLuaObject?
There was a problem hiding this comment.
enable = false should be kept, and enable should be filtered out before being passed to toLuaObject here. keep in mind that, the enable field is only used by nvf to call vim.lsp.enable('lspname'), and has no effect when passed to vim.lsp.config()
There was a problem hiding this comment.
ok my bad, there was another effort in fixing this already #1191
I'll read through the discussions in that again later, I kinda forgot about it.
I think it's best to just leave haskell-tools alone for now, I'll merge in the other changes
There was a problem hiding this comment.
repushed and removed the haskell commit
|
repushed with a rebase on current main |
|
this doesn't seem to be caused by my PR I think |
found these in
:checkhealthSanity Checking
nix fmt).#nix(default package).#maximal.#docs-html(manual, must build).#docs-linkcheck(optional, please build if adding links)x86_64-linuxaarch64-linuxx86_64-darwinaarch64-darwinAdd a 👍 reaction to pull requests you find important.